Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/improve revert reason #1727 #2018

Merged
merged 24 commits into from
Jan 23, 2020

Conversation

alant
Copy link
Contributor

@alant alant commented Dec 8, 2019

Fixes #1727

contracts/token/ERC721/ERC721.sol Outdated Show resolved Hide resolved
contracts/token/ERC721/ERC721.sol Show resolved Hide resolved
contracts/token/ERC721/ERC721.sol Outdated Show resolved Hide resolved
@frangio
Copy link
Contributor

frangio commented Dec 9, 2019

Thanks @alant! I left a few comments.

alant and others added 8 commits December 9, 2019 18:02
Co-Authored-By: Francisco Giordano <frangio.1@gmail.com>
…we want to ignore the first 4 bytes of content, so we should read the length and decrease it by 4, then take the memory location and add 4 to it, then store the new length at the new memory location, then that is the new byte array that we want.
test/token/ERC721/ERC721.behavior.js Outdated Show resolved Hide resolved
contracts/token/ERC721/ERC721.sol Outdated Show resolved Hide resolved
contracts/token/ERC721/ERC721.sol Outdated Show resolved Hide resolved
contracts/token/ERC721/ERC721.sol Outdated Show resolved Hide resolved
@frangio
Copy link
Contributor

frangio commented Jan 14, 2020

In f279bec I've fixed the bit of assembly code that was wrong.

With this fix things should be finally working correctly now, and you should be able to change the expectRevert.unspecified that you added into the proper expectRevert(..., 'ERC721ReceiverRevertsMock: Transaction rejected by receiver'). This ensures that the revert reason is being forwarded correctly.

Sorry you had to endure so much back and forth on this!

@frangio
Copy link
Contributor

frangio commented Jan 14, 2020

Also, I've noticed ERC721ReceiverRevertsMock and ERC721ReceiverMock are exactly the same. Can you remove one of them?

@alant
Copy link
Contributor Author

alant commented Jan 15, 2020

Also, I've noticed ERC721ReceiverRevertsMock and ERC721ReceiverMock are exactly the same. Can you remove one of them?

Done.

@alant alant closed this Jan 15, 2020
@alant alant reopened this Jan 15, 2020
@alant
Copy link
Contributor Author

alant commented Jan 15, 2020

In f279bec I've fixed the bit of assembly code that was wrong.

With this fix things should be finally working correctly now, and you should be able to change the expectRevert.unspecified that you added into the proper expectRevert(..., 'ERC721ReceiverRevertsMock: Transaction rejected by receiver'). This ensures that the revert reason is being forwarded correctly.

Sorry you had to endure so much back and forth on this!

No worries. I was not able to change the test to expectRevert(..., 'ERC721ReceiverMock: 'ERC721ReceiverMock: reverting')and have them pass. Can you take another look and let me know please? Thanks.

@frangio
Copy link
Contributor

frangio commented Jan 15, 2020

expectRevert(..., 'ERC721ReceiverMock: 'ERC721ReceiverMock: reverting') is not correct. The expected revert reason for the tests which use ERC721ReceiverRevertsMock is 'ERC721ReceiverRevertsMock: Transaction rejected by receiver'. Remember the point of this feature is to forward the original revert reason!

@frangio frangio mentioned this pull request Jan 20, 2020
2 tasks
@nventuro
Copy link
Contributor

Thank you for your contribution @alanarvelo!

@nventuro nventuro merged commit 7d7cbca into OpenZeppelin:master Jan 23, 2020
KaiRo-at pushed a commit to KaiRo-at/openzeppelin-contracts that referenced this pull request Mar 16, 2020
* adding mock contacts, test code

* adding changes to ERC721.sol per @frangio's comments on original PR OpenZeppelin#1943

* fix solhint warnings

* Update contracts/token/ERC721/ERC721.sol

Co-Authored-By: Francisco Giordano <frangio.1@gmail.com>

* same revert wording per @frangio's review suggestion

* per @frangio's feedback, changing the inline assembly to accomplish: we want to ignore the first 4 bytes of content, so we should read the length and decrease it by 4, then take the memory location and add 4 to it, then store the new length at the new memory location, then that is the new byte array that we want.

* change revert msg assembly per PR comment by @frangio

* unify revert msg in test code

* fix some failed tests, wording change

* Update contracts/token/ERC721/ERC721.sol

Co-Authored-By: Francisco Giordano <frangio.1@gmail.com>

* Update contracts/token/ERC721/ERC721.sol

Co-Authored-By: Francisco Giordano <frangio.1@gmail.com>

* fix test case, revert without reason

* fix 'ERC721ReceiverRevertsMock: Transaction rejected by receiver'

* style change per review by @frangio

* fix revert reason forwarding

* remove duplicate contracts/mocks/ERC721ReceiverRevertsMock.sol per review https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2018\#issuecomment-574381034

* Add changelog entry

* Fix tests

* Make tests more clear

Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
Co-authored-by: Nicolás Venturo <nicolas.venturo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve revert reason when assuming interface compliance
3 participants